Repository navigation
fix(aws-startup-advisor): conflict options apply the Sonnet 5.5 yield and exact pin matching in agent-advisor - #620
Conversation
… exact pin matching Follow-up to #607. _decision_options now applies _yield_to_capable_alternative and _honor_explicit_profile like the main path, so each option names the model a rerun on that path selects. Pin matching uses exact id segments, so a Sonnet 5.5 pin no longer names Sonnet 5 by prefix.
Pre-PR reviewAn independent read-only review (a Sonnet subagent) checked this diff before the PR was opened. Its findings and my responses:
The review also checked these and found them unchanged: |
|
Problem, Fix, and Scope are filled in. A change type is selected and all four Startup Advisor author confirmations are complete. The author checklist check now passes. CI and reviewers still need to verify the change. |
herosjourney
left a comment
herosjourney
left a comment
There was a problem hiding this comment.
Findings: 0 Blocking, 1 Should-fix, 0 Nits. Heads-up: 1.
Reviewed: head 9ff076e (base 09af68c, merges cleanly into main), 2 files (+101/−4) — agent-advisor (anthropic_model_recommendation.py, test_model_refresh_a2.py), one plugin tree.
Ran: mise -C plugins/aws-startup-advisor run ci → exit 0 (test 1717 passed, test:telemetry 311 passed, the agent-advisor task collects the changed test file); test_model_refresh_a2.py directly → 50 passed; four mutations of the head, each caught by a new test (revert the _decision_options change → 4 fail; revert only the pin matcher → 1 test fails; options without the pin → 2 fail; options without the yield → 3 fail); my own option-vs-rerun sweep, 7,168 options across 4 regions, 4 residency settings, 4 priorities, thinking on/off, Batch on/off, 7 pin shapes, Converse and Invoke paths → 0 mismatches at head, 3,310 on main; old-vs-new pin matcher over every catalog model id and 8 prefixes × 3 ARN/bare forms → only the two disclosed pin shapes change; full recommend() output diff main vs head over 32,256 inputs (details under the Should-fix).
Did not run: the author's 56,160-case golden comparison (I ran my own matrix above instead); no live AWS calls. The Startup Advisor author checklist check fails with "PR checklist incomplete". Its summary does not name the item that trips it, so I have not diagnosed it. It reruns when the description is edited.
Change narrative: conflict options offered each path's top-ranked candidate, skipping the Sonnet 5.5 capability yield and the inference_profile_id pin the main path applies → _decision_options now runs _yield_to_capable_alternative then _honor_explicit_profile, in the order recommend_anthropic_workload does at line 983–984; _pin_names_model replaces three substring checks, so global.anthropic.claude-sonnet-5-5 no longer names anthropic.claude-sonnet-5 → one engine file plus its tests. Body vs diff: matches on the code and the tests; the "Visible shift" paragraph describes one of the two ways selections move (see the Should-fix).
| Axis | Stance | One line |
|---|---|---|
| Parsimony | clear | 14 changed lines of engine code and one helper; the 87 test lines are proportionate to two bugs |
| Completeness | finding | The most common input shape that changes (a runtime pin, no path constraint) has no test and no mention in the body |
| Security | clear | scanners: CodeQL and dependency-review pass; contracts touched: none; the helper only parses a string |
| Alternatives | clear | none worth raising |
| Contradictions | clear | model-recommend.md Step 6 and its pin-validation paragraph describe the behavior at head |
| Overlap | advisory | #588 edits _decision_options and the same file; see Heads-up |
| Cross-plugin parity | n/a | one plugin tree |
| Propagation & data | clear | SSOT: the model catalog; sites swept: _pin_names_model has 1 caller (_honor_explicit_profile); stale forms found: 0 |
Should-fix
- [Completeness] A runtime pin on the unconstrained default path now flips
api_pathand the model, and no test or body text covers it —plugins/aws-startup-advisor/skills/agent-advisor/scripts/anthropic_model_recommendation.py:503if not _pin_names_model(explicit, path_config["model_id"]):
Consequence: for{priority: balanced, data_residency: global_allowed, inference_profile_id: "global.anthropic.claude-sonnet-5-5"}with no governance and nopreferred_api_path,mainreturnsmantle_messages/anthropic.claude-sonnet-5(the pin dropped, a different model), and head returnsruntime_converse/anthropic.claude-sonnet-5-5/global.anthropic.claude-sonnet-5-5. The versioned Haiku 4.5 runtime pin moves the same way,mantle_messages→runtime_converse. Both are the correct results. In my 32,256-input diff, 240 inputs flipapi_paththis way (all with no governance, no native payload, no preferred path), 704 more changeprimary_modelon Mantle, and 11,775 change onlydecision_options. The body says onlydecision_optionschange in the targeted matrix and describes the shift as "no longer selects Haiku 4.5 on Mantle". A team that pinned a runtime profile and had been getting a Mantle recommendation will now get a different integration path. The new tests pin the Mantle and the option outcomes. None pins this one (every_mantle_pinnedtest setspreferred_api_path).
Fix: add a sentence to the body's "Visible shift" paragraph: "With no path constraint, a runtime profile pin now selects the runtime path (runtime_converse) and the pinned model, wheremainreturned Mantle." Add the test below. Regression: it fails onmainand passes at head (I ran both).CI check: none.@pytest.mark.parametrize(("pin", "model"), [ ("global.anthropic.claude-sonnet-5-5", "anthropic.claude-sonnet-5-5"), ("us.anthropic.claude-haiku-4-5-20251001-v1:0", "anthropic.claude-haiku-4-5-20251001-v1:0"), ]) def test_a_runtime_pin_selects_the_runtime_path_when_no_path_is_constrained(pin, model): rec = _rec("anthropic", "claude-3-7-sonnet-latest", "messages", priority="balanced", inference_profile_id=pin) assert (rec["api_path"], rec["primary_model"], rec["invocation_model_id"]) == ("runtime_converse", model, pin)
Heads-up (out of diff, pre-existing — not blocking)
- #588 (approved, +4,051/−426, 10 commits behind
main) already conflicts withmainin this file andmodel-selection.md, and its_decision_optionsstill ends inoption = _candidate_summary(candidates[0], requirements, tradeoff, region)at its line 600, with a newruntime_messagesoption path and amantle_selectorargument. Whichever PR lands second has to carry this PR's yield-then-pin selection into that function, including theruntime_messagesoption. I can file a note on #588 to say so.
Verdict
Comment — nothing blocks the merge. The engine change is correct against every probe I ran. The one Should-fix is a disclosure and one regression test. The reviewing account is not the author's, and a Should-fix is open, so I do not approve.
leon1418
left a comment
leon1418
left a comment
There was a problem hiding this comment.
[🤖 AI review 🤖] Reviewed both changed files at 9ff076ea196f41ff33b80214ae42b800c7e8623d against base 09af68caa86c8913e60c4a66290b52c82e901d42, with OCR-assisted repository review, a fresh independent reviewer, and producer/consumer contract verification. No substantive findings remain; approval is recommended for the user to submit.
Local validation at this unchanged head: 653 agent-advisor tests and 415 llm-to-bedrock tests passed in the earlier attempt; this retry independently passed 286 focused tests, 1,440 parent and 2,440 independent option/rerun comparisons, and verification-retention/invalidation checks. Exact runtime pins and the unconstrained Sonnet 5.5 runtime-path selection were checked. No live AWS calls were made.
The existing nonblocking request for an unconstrained runtime-pin regression test and description clarification remains open for its original reviewer/author to address. This review does not resolve that conversation or establish merge readiness.
…no path is constrained From Logan's review on #620: with no path constraint, a runtime profile pin now selects runtime_converse and the pinned model, where main returned Mantle.
herosjourney
left a comment
herosjourney
left a comment
There was a problem hiding this comment.
Findings: 0 Blocking, 0 Should-fix open, 0 Nits. Heads-up: 1.
Re-review at 6321520 (previous: 9ff076e). Delta: 1 commit, 1 file (+12/−0, test_model_refresh_a2.py).
| Prior finding | Status at head |
|---|---|
[Should-fix] Unconstrained runtime pin changes the path, untested and undisclosed (anthropic_model_recommendation.py:503) |
Fixed at 6321520 — re-probed: the new parametrized test passes at head (2 passed) and fails on upstream/main (2 failed) with the test file copied across. The PR body's "Visible shift, unconstrained path" paragraph now states the change, and the matrix summary is corrected to 1,008 primary changes (360 unconstrained, 648 Mantle). |
New in delta: none. The delta touches only the test file, and the new test asserts the behavior the Should-fix named. Sweep for stale forms: not applicable, no value changed.
Ran: at 6321520, uv run python -m pytest -q test_model_refresh_a2.py → 52 passed (50 prior + 2 new). The new test against upstream/main → 2 failed. The PR body re-read for the disclosure and the corrected numbers.
Did not run: the 25,920-case matrix behind the corrected 1,008 figure. I re-read the description's numbers but did not re-derive them. The full mise -C plugins/aws-startup-advisor run ci was not re-run for this push; the prior ci exit 0 was at 9ff076e. No live AWS calls.
Heads-up (unchanged, not blocking): #588 still conflicts with this PR in the same engine file and has the older _decision_options. Whichever PR lands second has to carry the yield-then-pin selection into that function.
Verdict: Comment — the Should-fix is closed at 6321520, and nothing else is open on this PR. This supersedes my 2026-11-12 review (pullrequestreview-5867426850), which raised the Should-fix. The reviewing account is not the author's, so no identity line applies.
herosjourney
left a comment
herosjourney
left a comment
There was a problem hiding this comment.
Approving at 6321520. The Should-fix from 9ff076e is closed (re-review: pullrequestreview-5869207860), and no review thread is unresolved.
Ran: test_model_refresh_a2.py at 6321520 → 52 passed. The new unconstrained-pin test → fails on upstream/main, passes at head. CI at head: build, startup-advisor, checklist-test, CodeQL, dependency-review, pr-title and the author checklist all SUCCESS.
Not re-run by me at this head: the full mise -C plugins/aws-startup-advisor run ci (the build and startup-advisor checks on GitHub passed for 6321520), and the 25,920-case matrix behind the corrected description numbers.
Heads-up, not blocking: #588 conflicts with this change in anthropic_model_recommendation.py and carries the older _decision_options. Whichever PR lands second has to apply the yield-then-pin selection to that function.
Keep aws#588's _decision_options (runtime paths, mantle_selector) and apply aws#620's yield-then-pin selection to each option's candidates. Adapt aws#620's Mantle/default-path tests to the bedrock-runtime default (Mantle via mantle_selector; Messages source lands on runtime_messages) and add a runtime_messages option test. Verified: mise run ci in plugins/aws-startup-advisor passed (1857 tests).
Problem
Follow-up to #607 (stage 2 of #566). Gen's last review on #607 found one P2 that was not fixed before #607 merged.
When Messages continuity (
preserve_messages_api: true) conflicts with runtime-only governance such as Guardrails, the Anthropic module returnsdecision_requiredwith one option per API path._decision_options()offered each path's top-ranked candidate. It skipped the Sonnet 5.5 capability yield and theinference_profile_idpin, both of which the main path applies. So withthinking_enabled: falseor detected Message Batches, the runtime Converse option named Sonnet 5.5, but choosing it and rerunning selected Sonnet 5. The model shown was not the model the choice produces.Fixing that exposed a second, older bug in
_honor_explicit_profile. Pin matching used substring checks, so the Sonnet 5.5 profileglobal.anthropic.claude-sonnet-5-5also matched Sonnet 5's id. On a path without inference profiles (Mantle, where Sonnet 5.5 is not offered), a Sonnet 5.5 pin pulled Sonnet 5 ahead of the ranked choice. For example, at quality priority it replaced Opus 5.5.Fix
_decision_options()now picks each option the same way the main path does:_yield_to_capable_alternative, then_honor_explicit_profile, on that path's candidates. Each option now names the model a rerun on that path selects._pin_names_model(), replaces the substring checks. The pin's last path segment must equal the model id or end in.<model_id>._explicit_profile_matches()already used that rule for the later prefix and geography checks.Scope
skills/agent-advisor/scripts/anthropic_model_recommendation.pyand its tests intest_model_refresh_a2.py. No catalog, registry, or OpenAI module changes.main(56,160 cases): no changes.decision_options.runtime_conversewith Sonnet 5.5 and the pinned profile.maindropped the pin and returned Mantle with Sonnet 5.decision_optionschange; that matrix had been run before the pin fix was added.main, 1,356 options name a different model than the rerun. With this change, 0 do.runtime_converse) and the pinned model, wheremainreturned Mantle. A team that pinned a runtime profile and was getting a Mantle recommendation now gets a different integration path, which matches the pin.us.anthropic.claude-haiku-4-5-20251001-v1:0) no longer selects Haiku 4.5 on Mantle, whose model id is unversioned (anthropic.claude-haiku-4-5). That is the same prefix match this change removes, and a runtime profile pin has no effect on Mantle. The pin still selects Haiku 4.5 on the runtime path, and a test covers it.Tests
detected_featuresarray, and Batch infeature_status: the runtime option and the rerun both name Sonnet 5.runtime_converseand the pinned model. This is Logan's test; it fails onmain.Type of Change
Select at least one.
Startup Advisor review checklist
By submitting this PR, I confirm that:
mise -C plugins/aws-startup-advisor run ciand all checks pass.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.